-
-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minted filter for beamer and latex, safe for all writers #40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the things I'm a little concerned about, but overall I think this works pretty well!
If you would rather not have such a complicated filter that's OK too. But I think this would be a really helpful filter for anybody who wants to change the listings for beamer / tex 🙂
Stephen McDowell <notifications@github.com> writes:
+ -- NOTE: using "latex" here because using FORMAT as a parameter does
+ -- not seem to work. It should not cause problems for beamer.
+ return pandoc.RawBlock("latex", raw_minted)
Sorry, this one occurs in two places and I missed both in the last review. Why can't I do `return pandoc.RawBlock(FORMAT, raw_minted)`? If I do that then nothing shows up (aka the code blocks just disappear).
That's a bit of a rough edge in pandoc. "beamer"
doesn't currently work as a raw format. I've just
pushed a commit fixing that, but for now use "latex".
|
Ok the proposed final documentation is rendered here. I just need to add some tests later tonight and then it should be good (I'll add a new comment and remove When we think this is ready for merge, is it ok if I squash these all into one commit? The commit messages aren't very good, and the diff stack is unnecessary (I had some back and forth ...). I hesitate to do this because when I do I think GitHub removes the review comments with the questions I have above. But if they aren't actually problems, then I'll force push the badness away :) |
Great! Feel free to squash, but we can also squash this when merging (we generally do this for all PRs introducing new filters). |
ok i think this looks good 🙂 i hope the tests being written in python is ok, i had to do a lot of string manipulation and that's definitely my go-to language for that. grepping for the latex was annoying because latex syntax definitely conflicts with regex syntax. sorry for multiple force pushes :D but now it's just one commit, unless things need to change in which case future commits will be structured better and can just be squash merged on your end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
- elem.attributes preferred to elem.attr[3] - use `kind` as parameter name, overloading `type` makes debugging more difficult
update tests to make sure that `fragile` is not present in non-beamer output
make sure tests assert that `nil` is not present to ensure that metadata keys are not being processed incorrectly
Ok round two complete, thanks for the really helpful review on making this much cleaner @tarleb! All of your feedback has been incorporated and a few updates to the tests to actually validate the IIRC when squashed the title of the PR is what the main commit message is. I don't know what the main message should be, but feel free to edit it to be whatever you want. |
Thanks again! This is probably the best documented and tested filter in the collection. |
Woohoo, you're very welcome! Hehe yes ... I got a little carried a way xD |
Am impressed by how well this filter is written and documented! Dedication to detail at its best :-)! I hope this is the best place to leave this info for others: For the ArXiv preprints workflow, there are some limitations on using minted. In essence, it is important to use a non-hidden cache directory, like so
The important thing is that the Full details are here: https://arxiv.org/help/faq/mistakes#minted Disclaimer: I did not test this yet, but thought it is worth logging it for the future. |
Hey! I made a new years resolution to get this all nice and shiny and in
lua
, and I think it's looking pretty good so far.